BitGenerator support - #499
Conversation
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
This looks like a useful addition! Thanks for working on it. I'm definitely not an expert here, but I left a few comment about things that stood out to me. Let me know what you think.
Also, are there any differences between numpy v1 and v2 that we need to consider?
|
OK, all addressed! I removed the 3.9 gating as asked, so CI fails now, maybe I should put it back until the CI for 3.8 is removed? To have |
|
Thanks, I'll try to hopefully give this a final review in the coming week.
I've just landed #567 which removes 3.8 support, so a rebase/merge with main should fix CI.
I think we should leave seeding (and other additional features) to a followup. This is already a quite a big PR and we should try to land that first and continue from there. |
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
I think the implementation is good now. Just a few small suggestions, then this is good to go 🚀
Icxolu
left a comment
Icxolu
left a comment
There was a problem hiding this comment.
Thanks you very much for sticking with me here 🙏 . It took a while but we got there eventually.
|
@mejrs As you have previously looked at this. Do you also want to take another look or are you fine with me merging this? |
|
That was a long time ago 😊 At a glance it looks OK to me, I'm not going to review it further. I'm fine with it merging. |
|
Nice! I should have gone with your first intuition, which ended up being the end result anyway. Could have saved us some time, but at least I learned a lot. I was feeling bad for nudging you to save my bad design lol |
No worries. Sometimes you need to implement something too see how things play out and occasionally you need to scrap things again, that's just how it goes sometimes. I also learned a thing or two about numpy rngs, and since you also learned something I'd say it's time well spend. |
4 participants
See
Fixes #498
The idea is to have a safe wrapper around the
npy_bitgenstruct that implementsrand::RngCore. That way pyo3 functions could be passed anp.random.Generator, get that wrapper from it, and pass it to Rust APIs, which could then call its methods repeatedly.The way it’s implemented, the workflow would look like this:
castanp.random.BitGeneratorinstance into anumpy::random::PyBitGenerator..lock()on it to get anumpy::random::PyBitGeneratorGuard.TODO:
Safety
If somebody releases the threading lock of the
BitGeneratorwhile we’re using it, this isn’t safe 🤔API design options
I could make this more complex by adding a new trait that is implemented by both
PyBitGeneratorandPyBitGeneratorGuard, allowing to choose if someone wants toPyBitGenerator’srandom_*methods directly on that object while holding the GIL and without locking itnp.random.BitGeneratorand returning a GIL-free object that can be used.but for now I just implemented the use case that’s actually desired.